-
Notifications
You must be signed in to change notification settings - Fork 27
Space - Diana & Lee #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Customers Index Controller Action & Tests
Videos Index Controller Action and Tests
…and ChangeDateFieldsToDateDatatype
…ntals#check_in action
…elds are returned for a valid rental
…ncrements available_inventory, and sets the due_date to nil
…ustomer or video does not exist
… rental_data params
…ry, and update due_date logic to video model method
…s_checked_out_count is < 1
…o total_inventory
…e_inventory would exceed total_inventory
…ed due_date from rental_params
…ion-tests Customer and Rental Validations Tests
Video Validations Tests
Rental Check Out Tests and Action
Rental Check In Tests and Action
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Additional FeedbackGreat work! Y'all built a fully functional API! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| end | ||
|
|
||
| Rental.check_out(rental.customer_id, rental.video_id) | ||
| # For some reason, we needed to reload the customer and video in order to get the updated counts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I'd expect here. :)
You updated the values in the database but need to reload to see the new values reflected in the scope of the variables in this method.
| return | ||
| else | ||
| render json: { | ||
| errors: rental.errors.messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's actually no chance that the request could be bad at this point because the customer_id and video_id have already been verified on line 9. Hence you don't have a unit test that verifies this case.
| customer.reload | ||
| video.reload | ||
|
|
||
| rental.due_date = Time.now + 7.days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: This could have been nice to add to the check_out method as well
Assignment Submission: Video Store API
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
If you didn't get to the functionality the question is asking about, reply with what you would have done if you had completed it.
Reflection
/rentalsendpoint, we deduced that this could be a bridge entity between the Customer and Video entities, and that there would be a many-to-many relationship between Customer and Video through Rentals. We checked our assumptions about entity attributes against the seed data./customers&/videosendpoints? What does the time complexity depend on? Explain your reasoning.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.